Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Developer Tools page #2046

Merged
merged 14 commits into from
Oct 21, 2024
Merged

Conversation

bharateshwq
Copy link
Contributor

Description

Fixes #2035

Created basic wire frame design for how the developer tools should look like while keeping in consideration the design language of policy Engine
Let me know if there are changes required for this

This in no way is the final commit that i expect to be merged there are a lot of optimization, responsiveness tasks and restructuring to be

Screenshots

image

@bharateshwq
Copy link
Contributor Author

Hey @MaxGhenis ,
I have pushed a new commit can you have a look at it

@anth-volk
Copy link
Collaborator

@bharateshwq This is looking great! Thanks for your work on it. Could you just do the following on the design:

  • Pad the titles underneath the images more on the top, perhaps to about 1rem
  • Show how this looks on tablet (769 to ~1200px) and mobile (<769px) breakpoints

I might also ask for a couple changes to the text of the header afterward

@bharateshwq
Copy link
Contributor Author

Hey @anth-volk,
I have done the necessary changes and added responsiveness to the developer-hub page

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @bharateshwq, this is looking good. There are a few changes I was hoping to request:

  • At the moment, this code places a wrapper around the API status and simulations pages. Could you remove that, including the "back" arrow? The pages themselves should actually be unchanged.
  • Could you link this page into the footer with the tag "Developer tools"?
  • Minor, but could you add padding to the right of the text within the link boxes at the mobile viewpoint? In the image below, you can see how the word "Simulations" runs into the button.
Screen Shot 2024-10-02 at 5 57 36 PM

I might also request some textual changes, but am hoping to do that at the end.

.eslintrc.json Outdated
"allow": ["error", "warn", "info", "table"]
}
]
"jsx-a11y/no-static-element-interactions": "off"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this before your final commit?

@bharateshwq
Copy link
Contributor Author

Hey @anth-volk,

I'll definitely fix that. Do you prefer having the api-status route separate, or keeping it under developer-tools/api-status? Right now, it's the latter, in which case users might find it more intuitive to have a back button on the API status and simulation pages. Let me know your thoughts!

@anth-volk
Copy link
Collaborator

@bharateshwq I think the way you layered the URLs makes sense, but I also think their browser back button should be able to handle that navigation action, as I believe react-router-dom writes to history.

@bharateshwq
Copy link
Contributor Author

Yes @anth-volk, React Router does write to the history, so using the browser's back button would work for navigation. However, I'll be removing the back button from the UI. Additionally, the current(master) API status and simulation headers have different designs, which I feel may not appear consistent to the user.
How would you like me to proceed with this?

@anth-volk
Copy link
Collaborator

@bharateshwq That is a good point. Considering these are internal developer tools, I don't think it's too big of an issue, but certainly warrants opening a separate issue. I can't remember their designs well enough, but I'm guessing the API status page is more out-of-line with our current design. If you'd like to take it on after this one, we'd greatly appreciate it.

@anth-volk anth-volk self-requested a review October 4, 2024 17:09
@bharateshwq
Copy link
Contributor Author

Hey @anth-volk, I've made the updates as requested and am awaiting your feedback. Thanks!

@anth-volk
Copy link
Collaborator

@bharateshwq Thanks, I'm hoping to get to this either tonight or tomorrow

@anth-volk anth-volk force-pushed the developer-tools-page branch from 30fb351 to f5b5129 Compare October 7, 2024 21:09
Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @bharateshwq. Thanks again for your work on this, this will be a major benefit for internal development. I've left a couple of minor change requests, and also took the liberty of cleaning up a merge conflict/linting. After this, we should be good to merge!

@@ -61,6 +61,11 @@ function LinkSection() {
label: "Terms and Conditions",
isInternal: true,
},
{
link: `/${countryId}/developer-tools`,
label: "Developer tools",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label: "Developer tools",
label: "Developer Tools",

const { pathname } = useLocation();

useEffect(() => {
console.log("Scrolling to top on pathname change:", pathname);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log("Scrolling to top on pathname change:", pathname);

src/pages/DeveloperHome.jsx Show resolved Hide resolved
);
}

function ToolsStruct({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just personal preference: structs do exist in JS (albeit more as a concept) could you change this name?

src/pages/DeveloperHome.jsx Show resolved Hide resolved
src/pages/DeveloperLayout.jsx Outdated Show resolved Hide resolved
@bharateshwq
Copy link
Contributor Author

Hey @anth-volk ,
sorry for the late reply i have sent a new commit with all suggested changes

@anth-volk anth-volk self-requested a review October 15, 2024 18:49
Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this @bharateshwq, it's looking great.

Earlier, when I had asked that you remove the enclosing features, it was because it created a wrapper that added a "back" button and created two footers. We actually still want the "header" (the navbar) and the footer, just not the back button. Could you add the navbar back into these two pages?

After that, this will be good to go.

@bharateshwq
Copy link
Contributor Author

Hey,
i have made a new commit fixing this

@anth-volk anth-volk self-requested a review October 21, 2024 15:41
@anth-volk anth-volk force-pushed the developer-tools-page branch from ed70bca to f95945e Compare October 21, 2024 23:33
@anth-volk
Copy link
Collaborator

@bharateshwq I've taken the liberty of addressing some merge conflicts in the PR. I'll approve and merge once tests pass.

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @bharateshwq! Merging now.

@anth-volk anth-volk merged commit 3706197 into PolicyEngine:master Oct 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Create Developer Tools landing page
2 participants